Code Review: Graceful Error Handling

Summary

This PR improves error handling in the Ember client's vizzlyScreenshot function by introducing a custom VizzlyDiffError class and inverting the error handling logic. The approach is sound and aligns well with the goal of making Vizzly non-blocking for test suites.

Positive Aspects

✅ Clear intent: The VizzlyDiffError class makes the code's intent explicit—only intentional visual diff failures should break tests.

✅ Simple implementation: The solution is minimal and focused, avoiding over-engineering.

✅ Consistent with project philosophy: This change aligns with the client SDK philosophy from CLAUDE.md—keep the client thin and ensure Vizzly connectivity issues don't break user tests.

✅ Good documentation: The class and catch block include helpful comments explaining the rationale.

Issues & Concerns

1. Missing Test Coverage ⚠️

The Ember client (clients/ember/src/test-support/index.js) appears to have no test coverage. I couldn't find any tests specifically for this file. The changes introduce new error handling paths that should be tested:

• Test that VizzlyDiffError is thrown when failOnDiff=true and a diff is detected
• Test that network errors are caught and return {status: 'skipped', reason: 'error'}
• Test that JSON parsing errors are handled gracefully
• Test that the cleanup function still runs even when errors occur (already guaranteed by finally, but worth verifying)

Recommendation: Add a test suite for the Ember client similar to tests/sdk/client.test.js. Consider using a mock server approach like the integration tests in that file.

2. Inconsistency with Core Client SDK 🔍

The main client SDK (src/client/index.js, tested in tests/sdk/client.test.js:449-478) has different error handling behavior:

• On server errors (5xx), it auto-disables the SDK and returns null
• On connection refused, it auto-disables and returns null

The Ember client now returns {status: 'skipped', reason: 'error', error: message} for all non-diff errors and does not auto-disable.

Questions:

• Is this behavioral difference intentional? Should the Ember client also auto-disable on persistent errors?
• Should the return value format be consistent across both clients?
• The core client returns null on errors; the Ember client now returns an object. This could break user code that checks for truthy/falsy values.

Recommendation: Document this difference or consider aligning the behavior. If intentional, add a comment explaining why the Ember client doesn't auto-disable.

3. Return Value Breaking Change ⚠️

Previously, errors would be thrown (test fails). Now they return {status: 'skipped', reason: 'error', error: message}.

If users have code like:

if (await vizzlyScreenshot('test')) {
  // handle success
}

This will now evaluate to truthy even on errors (the returned object is truthy). This is unlikely but worth considering.

Recommendation: Document this as a potential breaking change in the PR description or consider returning null on errors to match the core SDK.

4. Error Message Visibility 💭

The error is logged via console.warn, which is good for debugging. However, in CI environments, console warnings might be missed. Users might not realize their screenshots are being skipped due to errors.

Recommendation: Consider if there's a better way to surface these errors in CI (e.g., summary at the end of the test run). This could be a future enhancement rather than blocking this PR.

5. Edge Case: VizzlyDiffError Constructor 🤔

The custom error class implementation is correct, but you might want to add Error.captureStackTrace for better debugging in Node.js environments:

class VizzlyDiffError extends Error {
  constructor(message) {
    super(message);
    this.name = 'VizzlyDiffError';
    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, VizzlyDiffError);
    }
  }
}

This is optional but improves stack traces by excluding the constructor itself from the trace.

Security Considerations

✅ No security concerns identified. The error handling doesn't introduce any new attack vectors.

Performance Considerations

✅ No performance impact. The try-catch adds negligible overhead, and cleanup still runs via finally.

Style & Conventions

✅ The code follows the repository's style conventions:

• Clear comments explaining the "why"
• Minimal implementation
• Follows existing patterns in the file

However, the PR title uses 🐛 (fix) which is correct according to CLAUDE.md's gitmoji guidance.

Recommendations Summary

1. High Priority: Add test coverage for this file, especially the new error handling paths
2. Medium Priority: Align error handling behavior with the core SDK or document why it differs
3. Low Priority: Consider Error.captureStackTrace for better stack traces
4. Discussion Needed: Decide on return value format (null vs object with error info)

Verdict

The change is conceptually sound and solves the stated problem. However, I recommend adding tests before merging to ensure this behavior is maintained and to catch any regressions. The inconsistency with the core SDK should also be addressed or documented.

Great work on improving the developer experience! The inverted logic ("only rethrow intentional errors") is much clearer than trying to catch every possible error type.